[db] MySQL: Set --explicit-defaults-for-timestamp=OFF #4226
Conversation
There was a problem hiding this comment.
Many thanks for fixing this @geropl! 💯
Code looks good to me, and fixes the problem:
-
SELECT @@GLOBAL.explicit_defaults_for_timestamp;now gives the correct value - I can manually start prebuilds in your deployment
I've just noticed a few inconsistencies around the ON UPDATE being present in our database but not in your new default params (not sure if it has an impact, but probably better to be 100% aligned with the DB). I've reported all inconsistencies as in-line suggestions.
| // DROP all input values as they are set by the DB 'ON UPDATE'/ as default value. | ||
| // We're relying on the system variable explicit-defaults-for-timestamp here (link: https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_explicit_defaults_for_timestamp): | ||
| // `undefined` get's converted to NULL, which on the DB is turned into the configured default value for the column. | ||
| // In our case, that's 100% CURRENT_TIMESTAMP. |
There was a problem hiding this comment.
Nit: I'd prefer being super-explicit here, in order to not leave any room for confusion
| // In our case, that's 100% CURRENT_TIMESTAMP. | |
| // In our case, that's 100% CURRENT_TIMESTAMP(6). |
There was a problem hiding this comment.
I think precision 6 we only have for part of them, no?
There was a problem hiding this comment.
| @@ -20,6 +20,7 @@ export class DBSnapshot implements Snapshot { | |||
| @Column({ | |||
| type: 'timestamp', | |||
| precision: 6, | |||
| default: () => 'CURRENT_TIMESTAMP(6)', | |||
There was a problem hiding this comment.
Shouldn't the ON UPDATE CURRENT_TIMESTAMP(6) also be part of this?
| default: () => 'CURRENT_TIMESTAMP(6)', | |
| default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)', |
There was a problem hiding this comment.
creationTime should only be updated on creation I guess, not when the row get's updated (that's what ON UPDATE CURRENT_TIMESTAMP does).
TBH I did not - but propably should have - cross-verified this with the actual DB columns but judged from the meaning....
There was a problem hiding this comment.
@geropl I'm happy to align this the other way around then, by removing the ON UPDATE CURRENT_TIMESTAMP(6) from the DB where you don't set them.
Would that make sense?
| @@ -31,6 +31,7 @@ export class DBPrebuiltWorkspace implements PrebuiltWorkspace { | |||
| @Column({ | |||
| type: 'timestamp', | |||
| precision: 6, | |||
| default: () => 'CURRENT_TIMESTAMP(6)', | |||
There was a problem hiding this comment.
Same question here:
| default: () => 'CURRENT_TIMESTAMP(6)', | |
| default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)', |
| @@ -33,13 +33,15 @@ export class DBAppInstallation implements AppInstallation { | |||
| @Column({ | |||
| type: 'timestamp', | |||
| precision: 6, | |||
| default: () => 'CURRENT_TIMESTAMP(6)', | |||
There was a problem hiding this comment.
Same question:
| default: () => 'CURRENT_TIMESTAMP(6)', | |
| default: () => 'CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)', |
Co-authored-by: Jan Keromnes <[email protected]>
Fixes #4169.
This PR contains two commits: